Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Split scroll between titles and folders for BM manager #8532

Merged
merged 1 commit into from
May 1, 2017
Merged

Split scroll between titles and folders for BM manager #8532

merged 1 commit into from
May 1, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Apr 27, 2017 •
edited by bsclifton
Loading

Test Plan:

  • Have a lot of bookmarks
  • Go to about:bookmarks
  • Ensure that:
    • Layout is fixed
    • You can scroll between folders (left-hand side column) and titles (right-hand side column) wouldn't scroll together
    • Less-than-screen-height number of bookmakrs shouldn't let you scroll (layout is fixed)
    • At small sizes we fallback to what we have and you can scroll gracefully

Description

@cezaraugusto cezaraugusto added this to the 0.15.1 milestone Apr 27, 2017
@cezaraugusto
Copy link
Contributor Author

/cc @bradleyrichter for design review. Scrollbar is the same used for #8475

@cezaraugusto cezaraugusto self-assigned this Apr 27, 2017
jonathansampson
jonathansampson suggested changes Apr 28, 2017
View reviewed changes
Copy link
Collaborator

@jonathansampson jonathansampson left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we prevent the Title and Last Visited headers from scrolling with the content? Also, thoughts on adding an overflow-ellipsis to the title entries?

bookmarks-headers

@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Apr 28, 2017 •
edited
Loading

@jonathansampson yep that's the ideal but I can't since it uses <table> (SortableTable component) which doesn't accept overflow.

Ideally sortableTable should make use of css-grid to keep it's table-like look, but changes would be too bulk and imo deserves a standalone refactor.

++ for ellipsis. I'll do another round on this since Brad don't want custom sidebars and include that, thanks!

@jonathansampson
Copy link
Collaborator

@cezaraugusto All valid points. I gladly approve, provided we can create an issue to refactor the bookmarks items portion for frozen headers.

@bradleyrichter
Copy link
Contributor

This is great to have, even with the unresolved header.

@cezaraugusto cezaraugusto mentioned this pull request May 1, 2017
Closed
@cezaraugusto
Copy link
Contributor Author

ok based on #8532 (comment) I created an issue to track fixed header for bm items: #8607. Also added ellipsis for long texts.

I also removed completely ScrollableContent element since component was created to address #7930 and this issue, and is no longer required.

bsclifton
bsclifton approved these changes May 1, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge ++ on this 😄 Looks and feels awesome

This functionality was originally there but it was lost when I refactored the page into its current state

@bsclifton bsclifton merged commit 4295f42 into master May 1, 2017
@bsclifton bsclifton deleted the bookmarks/5076 branch May 1, 2017 21:16
@CliqueBait CliqueBait mentioned this pull request May 2, 2017
Closed
@bsclifton bsclifton mentioned this pull request May 8, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathansampson jonathansampson jonathansampson requested changes

@bsclifton bsclifton bsclifton approved these changes

@luixxiul luixxiul luixxiul approved these changes

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@cezaraugusto cezaraugusto

Labels
feature/bookmarks
Projects
None yet
Milestone
0.15.300 (Frozen, only critical adds ...
Development

Successfully merging this pull request may close these issues.

Folders and Titles lists in about:bookmarks should scroll independently
5 participants
@cezaraugusto @jonathansampson @bradleyrichter @luixxiul @bsclifton